testBeforeNextSubmit is proving to be quite a decent way to test pings. However, it needs some help from the test writer to ensure things don't go awry. For example, this test will not fail:

add_task(async function test_should_fail() {
  GleanPings.myPing.testBeforeNextSubmit(reason => {
    Assert.ok(false);
  });
});

We've completely forgotten to actually submit the ping. This isn't great, so the new instrumentation testing docs being added in bug 1752357 recommend this pattern:

add_task(async function test_fails() {
  let submitted = false;
  GleanPings.myPing.testBeforeNextSubmit(reason => {
    submitted = true;
    Assert.ok(false);
  });
  Assert.ok(submitted);
});

This test will indeed fail if we forget to submit the ping. But it's an awkward manual process. Wouldn't it be nicer if it were more like:

add_task(async function test_times_out() {
  let submitted = GleanPings.myPing.testBeforeNextSubmit(reason => {
    Assert.ok(false);
  });
  await submitted; // Will wait forever if ping not submitted
});

Or maybe even some other better design we haven't yet come up with?

This ping is about designing and implementing a better ping testing design that makes it more ergonomic across the various SDK languages for instrumentation tests to assert that the ping was actually submitted and the testBeforeNextSubmit closure was called.

Note that this is the exact behaviour that Glean.js has (see here). The FOG JS API was quite different because submit was synch (if I remember right): not sure why we still need validatorRun there though

Another potentially-related point of interest: now that testBeforeNextSubmit can be called within testBeforeNextSubmit at least in FOG JS, we can use self-referential callbacks to capture multiple ping payloads in a row. At least one person thinks this is a pattern with value.

Perhaps this is something that'd dovetail with this work? Instead of "just" a submitted promise, it could be a submitted promise that resolves with the (full?!) ping payloads that were submitted?

Beth had an excellent idea of passing the fn that ought to submit the ping into the test API, perhaps like so:

GleanPings.pingName.test(aReason => {
  // assert ping contents as per usual `testBeforeNextSubmit()` callback.
}).beforeThisSubmits(() => theFnThatShouldSubmitThePing());

In JS this would run the beforeThisSubmits callback, run the test callback just before the next submit of "ping-name" triggered by the beforeThisSubmits callback, then throw an Error of some kind if the test callback wasn't called (ie, the ping didn't submit).

(( If the beforeThisSubmits callback is async, then the whole statement'll need await on it. ))

This would ensure that the test callback is called, solving the let submitted = false; dance. And supplies a stronger guarantee that the function we think is submitting the ping actually is submitting the ping instead of it happening somewhere/somewhen else (though we might have to be careful to check threading here if we're serious about it).

...but what about languages that don't throw, like C++ or Rust? What can they do if they detect that the submission didn't happen the way the callbacks said they would? We could crash, I suppose... but that's not a friendly thing to do, even in tests.

I mean, they don't have to support this form. They (and, heck, JS too) can keep testBeforeNextSubmit and keep up with the let submitted = false; dance. Or maybe we crash anyway, like what FOG does when test methods are called on child processes.

Backed out for causing build bustages in Ping.cpp

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/toolkit/components/glean/bindings/private/Ping.cpp:175:42: error: The fully qualified types are preferred over the shorthand typedefs for JS::Handle/JS::Rooted types outside SpiderMonkey.

https://hg.mozilla.org/mozilla-central/rev/529b0d1d2e58

https://hg.mozilla.org/mozilla-central/rev/a43e8e9563ab